Plane: ICE: fully move to aux function #28655
Conversation
There was a problem hiding this comment.
Regarding the loss of STARTCHN_MIN functionality: I know that the the control of the ICE is a crucial function. But doing away with this parameter can only mean inadvertent shutoff of the engine, not inadvertent start.
Under that light, is an inadvertent engine shutoff due to bad RC signals more dangerous than other events tied to AUX_FUNCTIONs?
E.g. switching to a different mode or enabling a parachute.
If we are concerned about minimum PWM values doing bad things, this is universal to all AUX_FUNCTIONs and we should do something about it for all of them in its their own library.
One more thing: You also need to modify the quadplane autotest MAV_CMD_DO_ENGINE_CONTROL; L1224 now fails because the AUX_FUNCTION being low doesn't block an engine start after a MAV_CMD_DO_ENGINE_CONTROL. Which I don't understand why, to be honest.
| @@ -163,7 +161,8 @@ const AP_Param::GroupInfo AP_ICEngine::var_info[] = { | |||
| // @Description: This is a minimum PWM value for engine start channel for an engine stop to be commanded. Setting this value will avoid RC input glitches with low PWM values from causing an unwanted engine stop. A value of zero means any PWM above 800 and below 1300 triggers an engine stop. To stop the engine start channel must above the larger of this value and 800 and below 1300. | |||
| // @User: Standard | |||
| // @Range: 0 1300 | |||
| AP_GROUPINFO("STARTCHN_MIN", 16, AP_ICEngine, start_chan_min_pwm, 0), | |||
|
|
|||
| // 16 was STARTCHN_MIN | |||
There was a problem hiding this comment.
Is it customary that we also delete the parameter description or no?
There was a problem hiding this comment.
Currently all the GCSs use param docs generated from master, so If we delete the description it doesn't show up in the GCS for older versions.
| uint16_t cvalue = 1500; | ||
| RC_Channel *c = rc().find_channel_for_option(RC_Channel::AUX_FUNC::ICE_START_STOP); | ||
| if (c != nullptr && rc().has_valid_input()) { | ||
| // get starter control channel | ||
| cvalue = c->get_radio_in(); | ||
|
|
||
| if (cvalue < start_chan_min_pwm) { | ||
| cvalue = start_chan_last_value; | ||
| } | ||
|
|
||
| // snap the input to either 1000, 1500, or 2000 | ||
| // this is useful to compare a debounce changed value | ||
| // while ignoring tiny noise | ||
| if (cvalue >= RC_Channel::AUX_PWM_TRIGGER_HIGH) { | ||
| cvalue = 2000; | ||
| } else if ((cvalue > 800) && (cvalue <= RC_Channel::AUX_PWM_TRIGGER_LOW)) { | ||
| cvalue = 1300; | ||
| } else { | ||
| cvalue = 1500; | ||
| } | ||
| } | ||
|
|
||
| bool should_run = false; | ||
| uint32_t now = AP_HAL::millis(); | ||
|
|
||
|
|
||
| // debounce timer to protect from spurious changes on start_chan rc input | ||
| // If the cached value is the same, reset timer | ||
| if (start_chan_last_value == cvalue) { | ||
| start_chan_last_ms = now; | ||
| } else if (now - start_chan_last_ms >= AP_ICENGINE_START_CHAN_DEBOUNCE_MS) { | ||
| // if it has changed, and stayed changed for the duration, then use that new value | ||
| start_chan_last_value = cvalue; | ||
| } | ||
|
|
||
| if (state == ICE_START_HEIGHT_DELAY && start_chan_last_value >= RC_Channel::AUX_PWM_TRIGGER_HIGH) { | ||
| if ((state == ICE_START_HEIGHT_DELAY) && (aux_pos == RC_Channel::AuxSwitchPos::HIGH)) { |
There was a problem hiding this comment.
This is a really nice cleanup!
This was originally added as a result of a dodgy RC system not going into failsafe but output all channels at 900. The result was the engine stopped and could not be re-started as there was no starter. All the other options you mention don't have to be on RC, until now RC was the only option for ICE start. |
I get that.
|
|
I'm for moving this to be more consistent with our other RC Channels options. I've always wondered why this one didn't get updated with the others.
That's not true, there's already the MAVLink command for engine start/stop. I use it regularly. Granted, there's no button to send that message in Planner currently. I've always done it through plugins. But I could add it to the actions dropdown. |
There was a problem hiding this comment.
We also loose the low PWM on RC failure protection of
STARTCHN_MIN.
that protection is essential. The combination of RFD900x with ICE is very common on large quadplanes, and it is still extremely hard to get this right due to the poor UI of the RFD900x. Making the ArduPilot code neater and more consistent is nice, but is not more important than protections like this.
|
@IamPete1 note that on aircraft of this type, the start chan is likely the only "aux" function on the aircraft. |
With this change they no longer need to setup the physical switch. So they can move to no aux functions at all. How would you feel about moving to a new aux function (with no param conversion), this would force users to acknowledge the change. The only other way round it is to move the min param to rc channels and apply it to all aux functions. Or we keep the existing behaviour and add a new aux function that the gcs can use. The key goal here is to expose the aux function to the gcs. |
Is the key goal to expose an aux function to the GCS, or to give the GCS the ability to start and stop the engine? ArduPilot/MissionPlanner#3451 I suppose the aux function allows you to set the engine to a state that ignores mission command engine kills and ignore Q_LAND_ICE_CUT, but I think that's actually a recipe for more confusion. Again, I'm actually in favor of having the aux function for this (and I think the min protection is not that essential), but I think GCSs should actually use the mavlink command, not the aux function, unless there's a good reason to do otherwise. |
|
No good options really, might try again in a year or two. |
…d high low thresholds
…input change before sending command
…eild for aux funciton triggers
|
I have moved to to the suggested trigger structure so AP_ICE can check the triggering PWM. There remain two issues:
|
| @@ -1410,7 +1418,7 @@ bool RC_Channel::run_aux_function(AUX_FUNC ch_option, AuxSwitchPos pos, AuxFuncT | |||
| // @Field: pos: switch position when function triggered | |||
| // @FieldValueEnum: pos: RC_Channel::AuxSwitchPos | |||
| // @Field: source: source of auxiliary function invocation | |||
| // @FieldValueEnum: source: RC_Channel::AuxFuncTriggerSource | |||
| // @FieldValueEnum: source: RC_Channel::AuxFuncTrigger::Source | |||
There was a problem hiding this comment.
Seems link the logger docs don't like two level deep enums.
This removes the ICE
STARTCHN_MINprotections and RC input thresholds instead moving over to those that are applied to all aux functions.Aux functions do have a min value of 800.
ardupilot/libraries/RC_Channel/RC_Channel.cpp
Line 1897 in 4d75b44
This also moves to the aux function threshold values of 1200 and 1800 rather than the 1300 and 1700 that are used now. ICE was also using a longer de-bounce time, 300ms vs 200ms in the RC lib.
This could mean a change in behavior for some users.
We also loose the low PWM on RC failure protection of
STARTCHN_MIN.We gain the ability to trigger the aux function directly from the GCS, so if RC dropout was a issue the recommendation would be to use the MAVLink command.
I don't really see any other nice way of moving forward. One option would be to add a
STARTCHN_MINparameter to the RC_Channel library and apply it to all switches.A second option would be to add another aux function that would be sent from the GCS and would be ignored if the existing option is setup. So you could do RC switch or GCS, but not both.